Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

packages/engine/internal/connector/kb_test.go (2)

64-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the metadata/metadatas mutual-exclusivity error.

kbMetadata explicitly errors when both metadata and metadatas are set (if hasSingle && hasMany { return ... "set only one of metadata or metadatas" }), but no test case exercises this branch. Given the surrounding table already tests the analogous content/contents conflict (line 71), consider adding a sibling case for the metadata param, and optionally one for vector/vectors both being set, since that ambiguity path isn't covered either.

♻️ Suggested additional test cases

 		{"count mismatch", map[string]any{"table": "t", "contents": []any{"a", "b"}, "vectors": []any{"[1]"}}},
 		{"metadata mismatch", map[string]any{"table": "t", "content": "a", "vector": "[1]", "metadatas": []any{map[string]any{}, map[string]any{}}}},
+		{"both metadata forms", map[string]any{"table": "t", "content": "a", "vector": "[1]", "metadata": map[string]any{}, "metadatas": []any{map[string]any{}}}},
+		{"both vector forms", map[string]any{"table": "t", "content": "a", "vector": "[1]", "vectors": []any{"[1]"}}},
 		{"sql injection in table", map[string]any{"table": "kb; DROP TABLE users;--", "content": "x", "vector": "[1]"}},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/engine/internal/connector/kb_test.go` around lines 64 - 85, Add test
coverage in TestPrepareUpsert_Errors for the kbMetadata mutual-exclusivity
branch by including a case where both metadata and metadatas are set, and assert
prepareUpsert returns an error. Use the existing table-driven pattern alongside
the content/contents conflict so the new case is easy to locate, and optionally
add a similar case for vector and vectors if you want to cover that ambiguity
path too.

---

137-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Approve; optional boundary case for top_k.

Error cases look solid (missing params, bad metric, negative top_k, injection). Since TestPrepareQuery_TopKCap verifies the upper clamp at 1000, consider also asserting behavior at the lower boundary (top_k: 0) to confirm whether it's treated as "use default" or rejected — currently untested.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/engine/internal/connector/kb_test.go` around lines 137 - 156, Add a
boundary test for `prepareQuery` to cover `top_k: 0`, since
`TestPrepareQuery_Errors` already checks negative values and
`TestPrepareQuery_TopKCap` covers the upper cap. Extend the existing
`TestPrepareQuery_Errors` table or add a dedicated case so it asserts the
intended behavior for `top_k` zero in `prepareQuery`, confirming whether it is
rejected or treated as the default.

packages/engine/internal/connector/kb.go (2)

24-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Identifier regex works well against injection, but blocks legitimate composite conflict targets.

kbIdentRe only matches a single (optionally schema-qualified) identifier. Applied to conflict_target at Line 165, this rejects any composite unique-key conflict target (e.g. "doc_id, chunk_id"), which is a common real-world case for dedupe keys spanning multiple columns.

Consider relaxing validation for conflict_target to accept a comma-separated list of valid identifiers (each individually checked against kbIdentRe), similar to how columns is handled at Lines 236-246.

♻️ Proposed fix to support composite conflict targets

 	var conflict string
 	if ct, ok := params["conflict_target"].(string); ok && ct != "" {
-		if !kbIdentRe.MatchString(ct) {
-			return "", nil, fmt.Errorf("conflict_target %q is not a valid SQL identifier", ct)
-		}
-		conflict = fmt.Sprintf(" ON CONFLICT (%s) DO NOTHING", ct)
+		parts := strings.Split(ct, ",")
+		for i, p := range parts {
+			p = strings.TrimSpace(p)
+			if !kbIdentRe.MatchString(p) {
+				return "", nil, fmt.Errorf("conflict_target %q is not a valid SQL identifier", ct)
+			}
+			parts[i] = p
+		}
+		conflict = fmt.Sprintf(" ON CONFLICT (%s) DO NOTHING", strings.Join(parts, ", "))
 	}

Also applies to: 163-169, 242-246

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/engine/internal/connector/kb.go` around lines 24 - 48, The
identifier validation in kbIdentParam is too strict for conflict_target and
blocks valid composite unique keys. Update the conflict_target handling in the
kb.go path that reads params so it accepts a comma-separated list of
identifiers, validating each piece with kbIdentRe instead of treating the whole
string as one identifier. Keep the existing single-identifier behavior for other
params, and mirror the list-splitting approach already used for columns.

---

186-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Error message omits documented metric aliases.

kbDistanceOperator accepts euclidean and ip as aliases for l2/inner_product, but the error message on Line 196 only lists cosine, l2, or inner_product. Minor inconsistency for users who pass an invalid metric and read the error to self-correct.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/engine/internal/connector/kb.go` around lines 186 - 198, The
kbDistanceOperator error message is missing the accepted alias names, so update
the default branch in kbDistanceOperator to mention all documented valid metrics
and aliases, including euclidean and ip alongside cosine, l2, and inner_product.
Keep the existing switch behavior unchanged; just adjust the fmt.Errorf message
so callers who pass an invalid metric can self-correct using the full set of
accepted values.

packages/site/src/content/examples/rag-kb-schema.sql (1)

32-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Only cosine has a matching index; l2/inner_product will sequential-scan.

kb/query's metric param supports l2 and inner_product, but this schema only builds a vector_cosine_ops HNSW index. Queries using the other metrics will silently fall back to a sequential scan instead of using an index.

📝 Suggested comment addition

 CREATE INDEX IF NOT EXISTS idx_kb_documents_embedding
     ON kb_documents USING hnsw (embedding vector_cosine_ops);
+-- Note: this indexes cosine distance only (kb/query's default metric).
+-- If you use metric: l2 or metric: inner_product, add a matching index
+-- (vector_l2_ops / vector_ip_ops) or those queries will sequential-scan.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/site/src/content/examples/rag-kb-schema.sql` around lines 32 - 33,
The kb_documents HNSW index only covers vector_cosine_ops, so kb/query requests
using metric values like l2 or inner_product can’t use an index. Update the
rag-kb-schema.sql example to either add matching HNSW indexes for the other
supported metrics or clearly note in the schema/comment that only cosine is
indexed; use the kb/query metric parameter and the existing CREATE INDEX on
kb_documents as the reference points.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes